-
Notifications
You must be signed in to change notification settings - Fork 2.2k
multi: add ability to fund+use musig2 channels that commit to a tapscript root #8546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In this commit, we rename the files as assembler.go houses the primary interfaces/abstractions of the package. In the rest of the codebase, this file is near uniformly called interface.go, so we rename the file to make the repo more digestible at a scan.
In this commit, we consolidate the root bucket TLVs into a new struct. This makes it easier to see all the new TLV fields at a glance. We also convert TLV usage to use the new type param based APis.
This'll allow us to create a funding output that uses musig2, but uses a tapscript tweak rather than a normal BIP 86 tweak.
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
In most cases, we won't yet be passing a root. The option usage helps us keep the control flow mostly unchanged.
This isn't hooked up yet to the funding manager, but with this commit, we can now start to write internal unit tests that handle musig2 channels with a tapscript root.
With this commit, the channel is now aware of if it's a musig2 channel, that also has a tapscript root. We'll need to always pass in the tapscript root each time we: make the funding output, sign a new state, and also verify a new state.
3176b9c to
fa13125
Compare
fa13125 to
33e0cea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, can really see the pieces coming together now 💯
|
|
||
| // TapscriptRoot is an optional tapscript root used to derive the | ||
| // musig2 funding output. | ||
| TapscriptRoot fn.Option[chainhash.Hash] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move this to next commit since it's not yet used here?
channeldb/channel.go
Outdated
| ), | ||
| } | ||
|
|
||
| if len(openChan.Memo) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fix the != 0 in this commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
| auxData.memo.WhenSome(func(memo tlv.RecordT[tlv.TlvType5, []byte]) { | ||
| tlvRecords = append(tlvRecords, memo.Record()) | ||
| }) | ||
| auxData.tapscriptRoot.WhenSome(func(root tlv.RecordT[tlv.TlvType6, [32]byte]) { //nolint:lll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: format as:
auxData.tapscriptRoot.WhenSome(
func(root tlv.RecordT[tlv.TlvType6, [32]byte]) {
tlvRecords = append(tlvRecords, root.Record())
}
)
instead? Or place the //nolint:lll above the line instead of at the end to make it even longer?
| op := wire.OutPoint{Hash: key, Index: uniqueOutputIndex.Load()} | ||
|
|
||
| var tapscriptRoot chainhash.Hash | ||
| copy(tapscriptRoot[:], bytes.Repeat([]byte{1}, 32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, should we also throw in a value for the memo since that doesn't seem to be covered yet?
| // don't have to provide for it during coin | ||
| // selection. The manually selected coins can be | ||
| // spent entirely on the channel funding. If | ||
| // selection. The manually selected coins can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just asking out of curiousity, did you change your editor's settings? Because it looks to me like this fit perfectly before...
| err error | ||
| ) | ||
| if chanState.ChanType.IsTaproot() { | ||
| fundingOpts := fn.MapOptionZ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice trick 😎
| func NewMusigPartialSig(sig *musig2.PartialSignature, | ||
| signerNonce, combinedNonce lnwire.Musig2Nonce, | ||
| signerKeys []*btcec.PublicKey) *MusigPartialSig { | ||
| signerKeys []*btcec.PublicKey, tapscriptTweak fn.Option[chainhash.Hash], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: break before tapscriptTweak instead?
| // tapscriptRoot is the root of the tapscript tree that will be used to | ||
| // create the musig2 funding output. This is only used for taproot | ||
| // channels. | ||
| tapscriptRoot fn.Option[chainhash.Hash] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused since it's already in partialState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had a look, beautiful usage of fn package
channeldb/channel.go
Outdated
| ), | ||
| } | ||
|
|
||
| if len(openChan.Memo) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this some unrelated old typo?
channeldb/channel.go
Outdated
| ), | ||
| } | ||
|
|
||
| if len(openChan.Memo) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
| revokeKeyLoc: tlv.NewRecordT[tlv.TlvType1, keyLocRecord]( | ||
| keyLocRecord{openChan.RevocationKeyLocator}, | ||
| ), | ||
| initialLocalBalance: tlv.NewPrimitiveRecord[tlv.TlvType2, uint64]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line length here and below
| anchorAmt: anchorSize * 2, | ||
| }) | ||
| }) | ||
| t.Run("taproot with tapscript root", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC these tests only check that the aux tapscript root can co-exist within the channel, but doesn't use the tapscript root in any way
correct?
| // DKeyLocator is a decoder for keychain.KeyLocator. | ||
| func DKeyLocator(r io.Reader, val interface{}, buf *[8]byte, l uint64) error { | ||
| // dKeyLocator is a decoder for keychain.KeyLocator. | ||
| func dKeyLocator(r io.Reader, val interface{}, buf *[8]byte, l uint64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't un-export these two functions, since we apparently use them in Loop.
|
@Roasbeef, remember to re-request review from reviewers when ready |
|
Replacement version of this PR with all current review comments applied and rebased on the |
In this PR, we add the initial plumbing that'll allow users to create and use musig2 channels that also commit to a tapscript root. Along the way, we start to store a new optional tapscript root in the
channeldb.OpenChannelstruct within the existing TLV stream appended to the older hard coded byte stream. The musig2 session itself will now conditionally pass in a tapscript root to the key aggregation and signing operations. We've also hooked up the lower half of the funding flow (lnwallet reservations) as well to ensure the new field gets propagated all the way down the relevant call stacks.